Add path exclusion to mTLS and BasicAuth authentication #151
Conversation
rzetelskik
force-pushed
the
path-exclusion-auth
branch
from
9705f1a to
ab8e6f5
Compare
|
@roidelapluie @SuperQ any chance to have someone review it? |
|
I don't know much about mTLS, but the title and description seem way too small for a +1200-line change. |
|
@bboreham I've added a section on implementation details, I hope it's clear enough. Also bear in mind that a bulk of that code are unit tests - I've tried to cover the changes quite extensively. The actual essence of these changes is only about 200-300 LoC, although I wouldn't really consider LoC a good indicator of complexity... |
Why? Maybe you are doing a large-scale refactoring? Please explain. It seems possible that this line should go higher up in the explanation:
|
Good point. I changed the main part of the description to include this.
I'm not sure I understand what your question is here. Why is the
I don't think I'm doing much refactoring in this PR besides moving the code responsible for basic auth authentication to a middleware, but I've explained above the reasons for doing so. Other than that I've tried to keep the changes in the existing code minimal, so I don't understand where this is coming from. |
OK, we have a different view on that. The whole project is currently 1823 lines. Thank you, the rationale to create a middleware and chain makes sense, it just wasn't in the description first time around. |
|
ping @SuperQ @roidelapluie |
1 similar comment
|
ping @SuperQ @roidelapluie |
|
I do not feel confident writing TLS code ourselves is benefitting for the project. |
|
First and foremost, I don't agree that this approach includes "writing TLS code" at all. As explained in the PR description, all it does is it lowers certificate requirements on handshake and delegates the required certificate verification to a middleware. And for the verification itself it uses functions from a standard library, which is nowhere near to actually implementing the protocol. And even the code used to verify the certificates comes from the stdlib implementation: https://cs.opensource.google/go/go/+/master:src/crypto/tls/handshake_server.go;l=858. Having said that, I believe you @roidelapluie were the one to suggest extending path exclusion to mTLS in one of the previous attempts to do so: #106 (comment), so I'd be happy to hear the approach you had in mind. |
|
@roidelapluie ping |
1 similar comment
|
@roidelapluie ping |
|
any update on this one ? |
My team still needs this for prometheus-operator and I'm happy to work on merging this. As you can see, the maintainers don't seem particularly interested though. |
|
any update ? |
|
I have added this for discussion at a future Prometheus Dev Summit |
|
Thanks @bboreham! The use cases behind it: [3], [4]. [1] - prometheus/prometheus#9166 |
|
Hey, sorry for the delay in getting around to this. As per the Dev Summit, I am picking up the review hat for this. Please bear with me as I work my way through it 😄 |
There was a problem hiding this comment.
Sorry for the long delay, it did take a while to get through everything with the care it deserves. Overall this looks good! Thank you for adding all the tests 🙏 they really helped me understand and trust what is going on.
My main ask is documentation, documentation, documentation. Both for the end user, at the "how and why to configure my exporter" level, and for users of the library. If we export it, someone will use it, and they need to know the semantics and what exactly happens when. This is particularly dicy around the TLS options – with no documentation in the toolkit to get them started, we cannot expect developers who use this to jump through and parse all the details of the Go TLS configuration options we are now exposing them to. In general, if we don't need to export something, I would err on the side of keeping it private – it's easy to later make things public when there's a clear use case, but hard to keep track of the big library surface we are opening up here.
Could you please rebase the PR?
|
@matthiasr thanks for the detailed review! I'll try to get to rebasing it and going through your comments in the following weeks, bear with me as it's been quite a while since I submitted the PR, so I'll probably have to reiterate on some of the decisions I made. |
rzetelskik
changed the title
rzetelskik
force-pushed
the
path-exclusion-auth
branch
from
ab8e6f5 to
54f7515
Compare
rzetelskik
force-pushed
the
path-exclusion-auth
branch
9 times, most recently
from
af8ba60 to
4220a93
Compare
rzetelskik
changed the title
|
I rebased the PR on top of the current master and addressed all the comments. As for exporting the @matthiasr please have a look |
There was a problem hiding this comment.
As for exporting the authentication package - perhaps we could just move it to internal/?
Oh yeah that's a good idea! Let's make anything that's not supposed to be used outside of the exporter-toolkit itself internal. Other than that, this looks ready to go for me!
Signed-off-by: Kacper Rzetelski <[email protected]>
Signed-off-by: Kacper Rzetelski <[email protected]>
rzetelskik
force-pushed
the
path-exclusion-auth
branch
from
4220a93 to
d67c526
Compare
|
@matthiasr thanks! I moved it to |
|
This is great, as Kamal (https://kamal-deploy.org/) uses |
|
@SuperQ is there anything you need from my side to merge this one? |
There was a problem hiding this comment.
One question, what about including additional paths in code?
For example, if I'm implementing an exporter with a health endpoint that I want to bypass auth, wouldn't I want to be able to do that by default so that users don't have to manually modify their local configs?
@SuperQ what would be a good place to to do this in your opinion? Without modifying the signature of |
|
What do you think @matthiasr? |
|
Hmm, it seems odd to me to add something to FlagConfig that's not, you know, a flag. If we add it to that, we should go the whole mile and expose this as a flag. Alternatively, if you want it to be configurable on Serve without forcing everyone to change their invocations, you could add it using a functional options pattern; since this uses a vararg |
|
I would also argue that this PR is already very large and complex, and because of that has been difficult to get merged. Adding support for default paths defined in exporter code seems like a well-contained, additional feature that we could add in a separate PR? |
Agreed, it seems to be an extension that can well enough go separately, in which case the approach most suitable for working with exporters directly could be discussed in a dedicated issue. |
This PR introduces support for path exclusion from authentication for both mTLS and BasicAuth.
The main issue here was allowing for excluding the requests, which request a URL matching an excepted path, from
mTLS authentication. This PR achieves this by lowering the requirements in TLSConfig and delegating the certificate verification to a dedicated middleware - an approach heavily based on how kubernetes/apiserver handles authentication and authorization. To match this approach and allow for path exclusion for both types of authentication, basic authentication is also moved to a dedicated middleware.
As discussed in prometheus/prometheus#9166, the need for this comes from kubelet, the Kubernetes node agent, not allowing the use of certificates or safely providing credentials for liveness/readiness probes. See also prometheus-operator/prometheus-operator#5419.
This PR also takes into account the comments in the previous attempts to add support for this: #106 and #70.
Fixes prometheus/prometheus#9166
Edit:
Some implementation details, as requested by @bboreham :
This PR introduces an
Authenticatorinterface.The
Authenticatemethod takes an*http.Requestand decides whether the given request passes authentication.The
func WithAuthentication(handler http.Handler, authenticator Authenticator, logger log.Logger) http.Handlermiddleware is also introduced. It authenticates every request and passes it to the next handler on success.The
ChainAuthenticatoris also implemented to conjunct multiple Authenticators. Its implementation ofAuthenticatefails to authenticate the request unless it authenticates against all chained Authenticators successfully.There are two instantiations of the interface implemented in this PR:
BasicAuthAuthenticatorandX509Authenticator.BasicAuthAuthenticatorisolates all the code responsible for basic auth authentication that was there before inwebHandlerwithout any changes to the logic.X509Authenticatorimplements the part of mTLS server handshake that is normally executed for connections withClientAuthhigher thanVerifyClientCertIfGiven(https://cs.opensource.google/go/go/+/master:src/crypto/tls/handshake_server.go;l=878).The crux of this PR is the following: one can't just exclude certain URLs from mTLS authentication. There's simply no access to this information at that level of encapsulation. Therefore
ClientAuthin tlsConfig is always capped atRequestClientCert, and the actual certificate verification is delegated to the x509 middleware (ab8e6f5#diff-ee4d3f619ee85cfe4114c969b9777f1936391e90d3198df35fc6f0145077badeR246).For the actual path exclusion an
Exceptorinterface is introduced.func WithExceptor(handler http.Handler, authenticator Authenticator, exceptor Exceptor, logger log.Logger) http.Handlermiddleware callsIsExceptedand checks, based on a requested URL, whether a request is excepted from authentication. If so, it passes it directly to the provided handler, otherwise the handler is preceded with the authenticator.